modules: runtime deprecate module.parent #33534

aduh95 · 2020-05-24T10:50:33Z

TL;DR: runtime warning when this module.parent is accessed from a module that have a nullish value for their module.parent (when a ESM imports it or when the module is run as the entry point of the process).

Currently, module.parent provides a way for CJS users to get which CJS modules required the current module first. This feature – not very useful by itself – is also used to make assumption about the program entry point. It is not rare to find packages that wrap their test code inside a if(!module.parent) condition, assuming that a falsy value means the module is run from CLI. The issue with this pattern is that the assumption doesn't work as soon as the module interacts with ESM or REPL.

With ESM being unflagged in current and LTS release lines, I predict this could become a real pain point for users trying to use ESM in Node.js.

Current issue

The issue I'd like to address is when a dependency is using the if(!module.parent) pattern to run its tests, and the test code gets executed for ESM users without their knowledge; those tests that may have side effects and make the end user code fail without obvious reason.

Action taken in this PR

~~set module.parent to a truthy value instead of undefined (in this PR, I'm using a dummy class UnknownModule).~~ that was moved to module: make module.parent truthy when loaded from ESM #37702.
add a runtime warning when module.parent is nullish, so it warns:
- library authors not to use this pattern.
- end user facing the bug that it may impact their code.

Objections and alternative solutions

Because the UnknownModule object created is effectively just an empty object, it doesn't have any property of the module object users may expect.
If this is not acceptable, we can keep the current behaviour (module.parent is undefined if the module was loaded by something that is not a CommonJS module).
Some packages may use the if(!module.parent) pattern in their CLI exposed file. That would lead to end users seeing a warning they have no control over. I don't think there would be a lot of packages affected by this, but that's a possibility.
If this is not acceptable, we can check if the file is inside a node_modules folder (similarly to what we do for Buffer constructor), or stick to the doc only deprecation for the time being.

Refs: #32217

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2020-05-24T14:10:17Z

Didn't the documentation deprecation for this just land the other day? Do we really need to do a runtime deprecation that quickly?

aduh95 · 2020-05-24T14:22:17Z

Do we really need to do a runtime deprecation that quickly?

This PR won't land until v15 at the soonest (which is due for October), and it can also be blocked until v16 or later if we think it's too soon.
I figured opening the PR sooner rather than later would give more time for contributors to give their thoughts on this topic. Apologies if that was inappropriate.

ljharb · 2021-03-10T15:12:41Z

lib/internal/modules/cjs/loader.js

+    getModuleParent.warned = true;
+  }
+  if (parent === undefined) {
+    return new (class UnknownModule {})();


why this new class instance instead of keeping the return value the same?

It should probably at least be cached if it’s truly necessary, otherwise it breaks the expectation that module.parent === module.parent.

aduh95 · 2021-03-10T18:02:41Z

I'm not sure if it's worth keep working on this: it's been open for a few months at this point: we haven't received any bug ticket complaining about module.parent interoperability with ESM, nor do I see a lot of thumbs up on this PR. Maybe it's just too rare of an issue to deserve a runtime deprecation after all.

ljharb · 2021-03-10T18:27:00Z

@aduh95 i suspect that's more likely due to the (expectedly) slow adoption curve of native ESM. module.parent should never have been used in the first place, and a runtime deprecation for it helps everyone, unrelated to ESM use cases.

ljharb · 2021-03-10T18:49:20Z

@aduh95 can you elaborate on why "UnknownModule" is needed?

aduh95 · 2021-03-10T18:52:26Z

@aduh95 can you elaborate on why "UnknownModule" is needed?

The idea is to return a truthy value, it could be something else. I'm open to better suggestions 😅

The reason I'd like it to be truthy is to cover the case where a package author has written test code like this:

if(!module.parent) { /* test code with potential side effects here */ }

Currently this runs the test code when the module is imported using the ESM loader, and can make the end user code fails when they switch from CJS to ESM.

ljharb · 2021-03-10T19:29:58Z

Sure, but don't we want that code to continue to fail, since module.parent is inherently broken?

aduh95 · 2021-03-10T21:11:03Z

Depends what you mean by "continue to fail", currently it doesn't really fail: the test code is being executed on a production environment, which may or may not introduce unwanted side effects – it could as harmless as printing Done. in the console, or it could create or remove files in the current working directory, or it could try to spawn a web server on an already bound TCP port. Just listing what I encountered myself, but the point is it could be anything, and is not a very great experience for the user.

I think we should keep in mind that keeping undefined or any falsy value would likely impact end users trying to migrate from CJS to ESM, not the package author using such pattern in one of their abandoned project.

That being said, if the consensus is to keep the current behaviour and simply add the runtime warning, I'll update the PR.

ljharb · 2021-03-10T22:09:24Z

Fair - tbh tho this seems like two separate PRs, and it might be useful to land/review/discuss them separately.

aduh95 · 2021-03-10T23:21:40Z

Fair - tbh tho this seems like two separate PRs, and it might be useful to land/review/discuss them separately.

Fair, I've opened #37702 for the UnknownModule bits.

targos · 2021-03-12T13:27:19Z

I have found that a very popular module accesses module.parent for a use case that cannot be replaced with require.main:

aduh95 · 2021-03-12T13:50:26Z

I have found that a very popular module accesses module.parent for a use case that cannot be replaced with require.main:

https://github.com/sindresorhus/import-fresh/blob/f1dddc06da4f1cdfb44a3aaba1f2c353c00dea02/index.js

https://www.npmjs.com/package/import-fresh

They shouldn't be impacted, the runtime warning is emitted only if the module is loaded from ESM or REPL (when module.parent == null).

targos · 2021-03-12T13:52:59Z

I understand. I just wanted to add this somewhere si it's taken into account. Is there a better issue/PR where I can post that message?

aduh95 · 2021-03-12T14:01:16Z

It was discussed originally in nodejs/modules#469, so maybe there?

legobeat · 2023-05-01T22:40:30Z

@aduh95 Will you be rebasing this on main or do you prefer that someone else reopens it?

aduh95 · 2024-05-05T08:22:23Z

Unlikely to ever happen, we can keep it as doc-deprecated.

addaleax added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 24, 2020

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18

ljharb reviewed Mar 10, 2021

View reviewed changes

aduh95 force-pushed the runtime-deprecate-module-parent branch from b030017 to 049057e Compare March 10, 2021 18:36

modules: runtime deprecate module.parent

df3b47f

aduh95 force-pushed the runtime-deprecate-module-parent branch from 049057e to df3b47f Compare March 10, 2021 18:39

aduh95 added the review wanted PRs that need reviews. label Mar 10, 2021

fixup! modules: runtime deprecate module.parent

557e41a

aduh95 mentioned this pull request Mar 10, 2021

module: make module.parent truthy when loaded from ESM #37702

Closed

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

legobeat mentioned this pull request May 1, 2023

Drop or patch dependency import-fresh eslint/eslintrc#110

Closed

aduh95 closed this May 5, 2024

aduh95 deleted the runtime-deprecate-module-parent branch May 5, 2024 08:22

Uh oh!

modules: runtime deprecate module.parent #33534

modules: runtime deprecate module.parent #33534

Uh oh!

Conversation

aduh95 commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current issue

Action taken in this PR

Objections and alternative solutions

Checklist

Uh oh!

mscdex commented May 24, 2020

Uh oh!

aduh95 commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

ExE-Boss Mar 10, 2021

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Mar 10, 2021

Uh oh!

ljharb commented Mar 10, 2021

Uh oh!

ljharb commented Mar 10, 2021

Uh oh!

aduh95 commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Mar 10, 2021

Uh oh!

aduh95 commented Mar 10, 2021

Uh oh!

ljharb commented Mar 10, 2021

Uh oh!

aduh95 commented Mar 10, 2021

Uh oh!

targos commented Mar 12, 2021

Uh oh!

aduh95 commented Mar 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Mar 12, 2021

Uh oh!

aduh95 commented Mar 12, 2021

Uh oh!

legobeat commented May 1, 2023

Uh oh!

aduh95 commented May 5, 2024

Uh oh!

Uh oh!

aduh95 commented May 24, 2020 •

edited

Loading

aduh95 commented May 24, 2020 •

edited

Loading

aduh95 commented Mar 10, 2021 •

edited

Loading

aduh95 commented Mar 12, 2021 •

edited

Loading